Looks good, I'll test this on my codebase a bit later. Thanks ! @johnfairh
Just realized i'm not a collaborator anymore so can't review this 🤔 weird

Just realized i'm not a collaborator anymore so can't review this 🤔 weird

Fiiiiiiixed.

One thing I want to make sure of is that A or its sub-declarations remain undocumented in the following situations:

/// :nodoc:
public struct A {
  /// some docs
  public let a: Int
}

And:

public struct A { // < no documentation comment
  /// some docs
  public let a: Int
}

Interesting!

The :nodoc: case behaves like that, encountering that immediately stops traversing the tree and does not include A in the docs.

The second case though includes A, calls it Undocumented in order to include the a field. Any members of A without doc comments are skipped. This was my best guess at what documented_child was intended to do (otherwise why bother with the routine / any kind of recursion).

Looking at the user issues again though I see they are entirely about extensions. So maybe the way you see this working is, with --skip-undocumented:

• As soon as we find an undocumented declaration, skip it + all its children regardless of their doc comments, so there are no Undocumented symbols;
• But make sure extensions with doc comments that do not say Undocumented count as 'documented'.

Sound right?

As soon as we find an undocumented declaration, skip it + all its children regardless of their doc comments, so there are no Undocumented symbols;

👍

But make sure extensions with doc comments that do not say Undocumented count as 'documented'.

I don't think that's what I'd expect. The doc comment in an extension should meaningfully count as documentation for that extension:

/// Extends `NSString` with functionality useful to this library
extension NSString {
  /// Some insightful docs into what `asdf` does.
  public func asdf() {}
}

I think that ideally --skip-undocumented would still generate docs for the NSString extension with an "Undocumented" description, as long as it has a documented_child. IMHO there's no point in adding that to the generated HTML if --skip-undocumented and it has no documented children. However, I'd still say that the extension should be included in the undocumented count.

Does that make sense?

The doc comment in an extension should meaningfully count as documentation for that extension

That seems counterintuitive to me, @jpsim. I follow @johnfairh’s logic more easily.

I was never even sure Swift itself intended to support markup for extension at all. I went to the official documentation looking for an answer.

The word used consistently throughout the documentation is “symbol”.

Use markup [...] to show Quick Help for your Swift code symbols.

The rough understanding in my mind, having never looked it up, was that a computing symbol is a source code entity that is referenceable and unique (within its scope).

Searching for “symbol (computing)” at Wikipedia redirects to “Symbol Table” where it says:

[A] symbol table is a data structure [...] where each identifier (a.k.a. symbol) in a program’s source code is associated with information relating to its declaration [...] in the source.

Following the “identifier” link to its page yields:

An identifier is a name that identifies [...] a unique object.

I’m not sure if the writers of the documentation were concerned with this level of specificity, but by that definition, extensions are not symbols. They have no names and there is no way to reference them, query them, store them in variables, tell them apart, or even tell if the symbols they contain are part of an extension or not. Most significantly, they do not have a unique declaration—that is there purpose, after all, to add functionality to an existing symbol without declaring any new entity.

Normally documentation belongs at the declaration (because it is unique and easily discoverable), not the usage sites. This example is a good illustration:

/// A variable.
public var variable: String?

func setVariable(to value: String) {

    /// The variable can be set. (Isn’t this documentation misplaced?)
    variable = value
}

By the same logic, documentation would belong at the type’s declaration, not its extensions.

/// A type.
public struct Type { ... }

/// Types can also do things. (This seems misplaced to me as well.)
extension Type { ... }

I understand that when a symbol is imported from another module and its declaration is out of the developer’s control, it can be nice to have a fallback location to note any additions, so I agree with using documentation where the user has provided it on an extension (even if Swift never does). However, considering extensions as things that should always be documented leads to absurd situations:

/// Some useful extensions.
extension ExternallyDefinedClass { ... }

/// Some more useful extensions.
extension ExternallyDefinedClass { ... }

extension ExternallyDefinedClass { ... }

extension ExternallyDefinedClass { ... }

/// Fancy stuff under certain conditions.
extension ExternallyDefinedClass where Component : Equatable { ... }

extension ExternallyDefinedClass where Component : Comparable { ... }

In this example:

• Is the developer doing something wrong?
• Which documentation comment should Jazzy output?
  • Should they be combined? How? In what order, especially if they are in separate files?
  • Should they be kept separate from one another? What should they be named in the navigation bar? How will the reader find what they are looking for, especially when things that conceptually belong together have to be split up because of constraints?
• How many “undocumented” warnings should be produced?

I like the “undocumented” warnings when they tell me that there is something exposed to users that they cannot look up. I do not like the idea of “undocumented” warnings for distinctions that do not actually exist for the user (such as the differences between various extensions and the corresponding declaration).

@jpsim yep on the same page, my second bullet was unclear even/especially after edit. Will revise code.

I'd like to leave any 'change the rules about requiring docs on extension declarations' out of this PR -- @SDGGiesbrecht these are some good questions, the middle ones apply today and are most important to the end user (reader), although I think relatively rare in practice.

Um... when I first posted, I thought this conversation was in #819, where @johnfairh and I had already been talking about something similar, and I feared this was a reversal of decisions made there. I just realized this is a different pull request. (Silly me.)

When I look closer I see that #819 has already been merged. My new understanding (after reading all the referenced issues) is that this pull request is really about bringing --skip-undocumented in line with #819. I’m all for that.

I was going to ask if the two (this and #819) could share logic in order to stay in lockstep in the future, but since you, @johnfairh, wrote both, I assume you already know how they interact. (I mention it for only for the sake of anyone else that is reading.)

I'd like to leave any 'change the rules about requiring docs on extension declarations' out of this PR

Sounds good.

Here is a summary what my expectations would be:

// ••••••• ••••••• ••••••• ••••••• ••••••• ••••••• •••••••
// Local Declarations (Structures, Classes, Enumerations, Protocols, etc.)
// ••••••• ••••••• ••••••• ••••••• ••••••• ••••••• •••••••

/// Documentation (→ Display)
public struct A {
    
    /// Documentation (→ Display)
    public func a() {}

    public func b() {} // No documentation → Skip (& warn)
}

public struct B { // No documentation → Skip (& warn)
    
    /// Documentation (→ But undocumented parent → Skip)
    public func a() {}

    public func b() {} // No documentation → Skip (& warn)
}

// ••••••• ••••••• ••••••• ••••••• ••••••• ••••••• •••••••
// Extensions of Local Types
// ••••••• ••••••• ••••••• ••••••• ••••••• ••••••• •••••••

/// Documentation (→ But declaration already has documentation → Merge (strategy undefined))
extension A /* Same module */ {
    
    /// Documentation (→ Display (merged into declaration method list))
    public func c() {}

    public func d() {} // No documentation → Skip (& warn)
}

extension A /* Same module */ { // No documentation → Parse children (no warning)
    
    /// Documentation (→ Display (merged into declaration method list))
    public func e() {}

    public func f() {} // No documentation → Skip (& warn)
}

/// Documentation (→ Declaration was skipped → Skip (or undefined))
extension B /* Same module */ {
    
    /// Documentation (→ But parent declaration was skipped → Skip (or undefined))
    public func c() {}

    public func d() {} // No documentation → Skip (& warn)
}

extension B /* Same module */ { // No documentation → Declaration was skipped
// → Parent may be undocumentable Core Data declaration
// → Display (or skip if detectably not Core Data, or undefined)
// (No new warning (already warning at declaration))
    
    /// Documentation (→ But parent declaration was skipped
    /// → Parent may be undocumentable Core Data declaration
    /// → Display (or skip if detectably not Core Data, or undefined))
    public func e() {}

    public func f() {} // No documentation → Skip (& warn)
}

// ••••••• ••••••• ••••••• ••••••• ••••••• ••••••• •••••••
// Extensions of External Types
// ••••••• ••••••• ••••••• ••••••• ••••••• ••••••• •••••••

/// Documentation (→ Display (merge strategy for multiple extensions undefined))
extension C /* Different module */ {
    
    /// Documentation (→ Display)
    public func c() {}

    public func d() {} // No documentation → Skip (& warn)
}

extension D /* Different module */ { // No documentation
// → Display (without “Undocumented” description) → Parse children (no warning)
    
    /// Documentation (→ Display)
    public func e() {}

    public func f() {} // No documentation → Skip (& warn)
}

I updated my last comment to fix some typos and oversights. I mention it here, because I doubt notifications contain the fixes. If you are reading the original in a notification, some details were not what I intended.

Also, not all of what I suggested above should necessarily be solved in this pull request. As long as this pull request makes steps towards that ideal and not farther from it, then the I support the pull request as is.

Pushed commit to wind this back a little: PR now is for extensions of external types like UIView in --skip-undocumented mode to display documented children, fixing problems reported in #502. As part of this, extensions that end up not containing anything are dropped from docs.

(one note on examples append above: Core Data default codegen is the example real-world use case in my mind for documenting the B extensions: user has no control over the class decl + the codegen does not supply a doc comment there. (For extra-super clarity, PR does not address this.))

Core Data default codegen is the example real-world use case in my mind for documenting the B extensions: user has no control over the class decl + the codegen does not supply a doc comment there.

Good catch, I’ll edit the list to note it in place.

Would it be worth adding those examples to the test suite?

Each one could be documented with either:

/// This should be visible, because...

or

/// This should be hidden, because...

Then whenever one appears or disappears in the output diffs, it is easy to see whether the change is wanted or a regression. It would also have the complete rationale right there in the diffs so it would be harder to overlook something when future changes or decisions are made.

Actually, I just convinced myself. I will go ahead and do that. If you think it is a bad idea or what to discuss the details, you can do so in the coming pull request.

You have my support for this already as‐is. I like the results when I test it. Thank you for working on it, @johnfairh.

Sorry for the delay, this is a lovely improvement. 👏

I'll merge realm/jazzy-integration-specs#32, update this PR to point to the merged commit, re-run this on CI to make sure it still passes, will fix it up if something broke in the months since this was done and get merged.

No worries - thanks. Big 👍 on the spec differ change.